Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dashboard): stackgraph loses ws status when drawn #791

Merged
merged 1 commit into from
May 29, 2019

Conversation

benstov
Copy link
Contributor

@benstov benstov commented May 22, 2019

Symptom: The dashboard stack graph resets when I click a node. For example, if a test is broken, so a node has a red border, and I click it, the color goes back to green. Same applies if I click something while some nodes are pending/processing.

Fix: merged the event status into the graph object (in graph container) being passed to the graph component(graph/index)

@benstov benstov requested a review from eysi09 May 22, 2019 15:27
Copy link
Collaborator

@eysi09 eysi09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Only thing that needs fixing is just the commented out code and the console log.


// // Here we"re setting nodeclass, which is used by our custom drawNodes function
// // below.
// if (nodeEl) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this.

// tslint:disable-next-line: no-invalid-this
const element = this as HTMLElement
if (element) {
clearGraphNodeSelection()

console.log(element.classList)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove console.log

@@ -366,24 +364,6 @@ class Chart extends Component<Props, State> {
}
}

// Update the node class instead of re-rendering the graph for perf reasons
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally added this because the websocket messages would trigger a re-render of the graph which, IIRC, was slow and choppy. But now it looks just fine! What's the secret?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the function getNodeClass inside the drawChart and we only rendering it if only the graph has changed (by changed i mean changed its ref - look at graph.tsx there is this code:
if (message && message.type === "event") { const nodeToUpdate = graph.data.nodes.find(node => node.key === (message.payload && message.payload["key"])) if (nodeToUpdate) { nodeToUpdate.status = message.name graph.data = {...graph.data} } }
changing the graph.data ref will trigger the drawChart.

im not so sure what is the secret, i just think that its more logical this way and maybe the god of javascript like this more so he decided to pump up the performance xD

@@ -42,6 +42,13 @@ export default () => {
if (!config.data || !graph.data || config.loading || graph.loading) {
return <Spinner />
}
if (message && message.type === "event") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good solution! The container is exactly the one that should be responsible for preparing the data for the component in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! :)
That's exactly the thought that guided me to this solution

@eysi09
Copy link
Collaborator

eysi09 commented May 22, 2019

Also, npm run fix-format to fix that CI issue.

@benstov benstov force-pushed the persistent-stackgraph-status branch from 0acb0c7 to 6ac5116 Compare May 23, 2019 11:24
@benstov benstov requested review from eysi09 and 10ko May 28, 2019 15:05
@eysi09 eysi09 force-pushed the persistent-stackgraph-status branch from 6ac5116 to 112e582 Compare May 29, 2019 09:39
@eysi09 eysi09 merged commit a03cafb into master May 29, 2019
@edvald edvald deleted the persistent-stackgraph-status branch June 30, 2019 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants